IMO we should completely decouple eval from the runs. This means, we should write run results to a specific file before invoking the success criteria. That way, custom criteria can look at everything. I.e; the folder structures can be standardized.

Review: coder_eval — pr:35

Scope: pr:35 · branch feat/run-command-pass-context · ba600b6 · 2026-07-23T04:00Z · workflow variant

Change class: complex — adds a pass_context data-exposure path (serializes the in-flight EvaluationResult / task.json schema to $CODER_EVAL_CONTEXT for scoring scripts) plus sandbox PATH hardening (node_modules/.bin); security-sensitive env/PATH handling and new criterion-field semantics require correctness reasoning.

Clean, well-tested feature work — pass_context for run_command plus sandbox node_modules/.bin PATH hardening — with zero blockers (no 🔴/🟠) across all 8 axes. The one substantive issue is a 🟡 reproducibility bug: in simulation dialog (every_turn/both) mode the pass_context payload leaks the previous turn's real weighted_score because _serialize_context scrubs only success_criteria_results, breaking the documented "weighted_score is null — do not read" contract; a doc/model mismatch (duration_seconds documented as null but a non-nullable float defaulting to 0.0) and a few test-coverage gaps round out the list. Overall 9.9/10; weakest axis Evaluation Harness Quality at 9.4/10.

Summary

Overall Score: 9.9 / 10 · Weakest Axis: Evaluation Harness Quality at 9.4 / 10
Totals: 🔴 0 · 🟠 0 · 🟡 1 · 🔵 4 across 8 axes.

Blockers

None.

Non-blocking, but please consider before merge

1. [Axis 8] pass_context scrub leaks stale prior-turn weighted_score in simulation dialog mode (src/coder_eval/criteria/run_command.py:35) — _serialize_context scrubs only one provisional field: return run_result.model_copy(update={"success_criteria_results": []}).model_dump_json(). The docstring and the pass_context field docs both promise that weighted_score is null and "do not read" it. That holds on the single-shot path (calculate_weighted_score runs AFTER check_all), but in simulation every_turn/both mode _run_dialog_criteria_check (orchestrator.py) calls self.result.calculate_weighted_score(...) at the end of EVERY turn, so at the next turn's check run_result.weighted_score carries the PREVIOUS turn's real score. A pass_context scoring script that reads weighted_score in dialog mode therefore gets a stale, non-null prior-turn value — a non-reproducible score for identical current-turn agent output. The scrub is inconsistent with itself: the exact "still holds the previous turn's results" reasoning cited to clear success_criteria_results applies equally to its derived weighted_score. Fix: extend the update= dict to also reset the post-check provisional fields (weighted_score=None, completed_at=None, duration_seconds=0.0) so the serialized payload matches its documented contract in all run modes.

Nits

1. [Axis 1] Resolved EvaluationResult named result, forcing awkward r loop variable in check_all (src/coder_eval/evaluation/checker.py:150) — In both check (line 150: result = run_result if run_result is not None else self._run_result) and check_all (line 188: same assignment), the resolved in-flight EvaluationResult is bound to a local named result. In check_all this collides with the codebase's convention of naming a CriterionResult result, forcing the per-iteration result to be renamed to the terse r (line 191: r = self._check_single(...); results.append(r)), which reads worse than the pre-PR result. Name the resolved value run_result (or run_result_val) to keep the loop variable as the conventional result and make it obvious the local is an EvaluationResult, not a CriterionResult. Purely cosmetic; no behavior change.
2. [Axis 3] Orchestrator wiring of pass_context (run_result=self.result) has no direct test (src/coder_eval/orchestrator.py:1355) — The three production seams that actually make pass_context work — orchestrator.py:1355, :1415, :1509, each run_result=self.result, passed into checker.check_all(...) — are exercised by no test. Every pass_context test drives SuccessChecker.check/check_all directly (tests/test_pass_context.py, tests/test_run_command_stdout.py::TestPassContextChecker), so if a future edit dropped run_result=self.result from any of the three orchestrator call sites, every pass_context=true criterion would silently raise ValueError and score 0.0 in production with no failing test. Add one orchestrator-level test (e.g. run check_success with a pass_context run_command criterion and assert the script saw a populated context) so the wiring seam is guarded. Severity kept Low because the downstream contract is very well covered one call away; only the trivial attribute-passing is untested.
3. [Axis 3] Payload 'placeholder/null' field contract asserted for success_criteria_results only, not the other documented fields (tests/test_run_command_stdout.py:440) — The pass_context Field docstring (models/criteria.py:229-236) makes an explicit contract that scripts rely on: weighted_score / completed_at / duration_seconds are null and final_status is still a placeholder ... do not read them. Only success_criteria_results == [] is asserted (test_payload_success_criteria_results_always_empty:458 and the round-trip test at line 440, which checks rehydrated.task_id == 'test' and rehydrated.iterations == []). No test asserts the payload's weighted_score / completed_at / duration_seconds are None — so a future change that finalized those before the criteria phase would silently expose values scripts were told to ignore, with no failing assertion. Add an assertion in the round-trip test that these fields are null in the serialized payload.
4. [Axis 8] pass_context docs claim duration_seconds is null but it defaults to 0.0 (non-nullable float) (src/coder_eval/models/criteria.py:236) — The pass_context field description states "weighted_score / completed_at / duration_seconds are null" (criteria.py:235-236), and docs/TASK_DEFINITION_GUIDE.md repeats "weighted_score, completed_at, and duration_seconds are null". But EvaluationResult.duration_seconds is declared duration_seconds: float = Field(default=0.0, ...) (models/results.py:446) — a non-nullable float that is 0.0 at check time, never null. A script author who trusts the doc (e.g. if ctx["duration_seconds"] is None) will misread the payload. Fix the field description and the guide to say duration_seconds is 0.0 (not finalized) rather than null.

What's Missing

Daily/nightly:

• 🟠 The PATH change (prepend -> append of /node_modules/.bin in _build_run_command_env) alters binary resolution on the production/nightly scoring run path: for identical agent output, any run_command whose command name collides with a binary present in node_modules/.bin (e.g. a package-local tsc/eslint or an agent-planted shim) previously resolved node_modules first and now resolves the venv/host binary. The PR does not state nightly blast radius — it can flip a task's score under identical agent output. Confirm no nightly task relies on node_modules/.bin shadowing and state the impact explicitly. (trigger: src/coder_eval/sandbox.py)
• 🔵 The PR does not state that pass_context is opt-in (default false) and that the serialized context is task.json-shaped with NO new persisted field, so the cross-repo contract (task.json schema / report JSON consumed by the external coder-eval-uipath eval-runner) is unchanged. This is the reassuring half of the blast-radius answer and should be recorded so a reviewer isn't left to infer it. (trigger: src/coder_eval/models/criteria.py)

Parallel paths:

• 🟡 _serialize_context was written for the single-shot path (where calculate_weighted_score runs AFTER check_all) but the parallel simulation every_turn/both path (_run_dialog_criteria_check calls calculate_weighted_score at the end of every turn) was not accounted for — the scrub clears success_criteria_results but not the per-turn-recomputed weighted_score, so the dialog path diverges from single-shot and leaks a stale prior-turn score. The scrub needed updating for the parallel path and wasn't. (trigger: src/coder_eval/criteria/run_command.py) (restates: Axis 8: pass_context scrub leaks stale prior-turn weighted_score in simulation dialog mode)

Tests:

• 🟡 No test exercises pass_context in simulation/dialog every_turn mode — the exact path where the stale-weighted_score leak lives. test_pass_context.py has only single-shot/direct-checker tests, so the dialog divergence ships unguarded. Add a dialog-mode test that asserts the serialized context's weighted_score is null at turn N+1 regardless of turn N's score. (trigger: tests/test_pass_context.py) (restates: Axis 8: pass_context scrub leaks stale prior-turn weighted_score in simulation dialog mode)
• 🔵 The three orchestrator seams that actually wire pass_context (run_result=self.result at orchestrator.py:1355/1415/1509) have no direct test — every pass_context test drives SuccessChecker.check/check_all directly, so dropping run_result=self.result from any call site would silently make every pass_context criterion raise ValueError and score 0.0 in production with no failing test. Add one orchestrator-level test guarding the wiring seam. (trigger: src/coder_eval/orchestrator.py) (restates: Axis 3: Orchestrator wiring of pass_context (run_result=self.result) has no direct test)
• 🔵 The payload null/placeholder contract promised in the pass_context field docs (weighted_score / completed_at / duration_seconds provisional, final_status a placeholder, do not read them) is only asserted for success_criteria_results == []. A future change finalizing the other documented fields before the criteria phase would expose values scripts were told to ignore with no failing assertion. Add round-trip assertions covering the other documented fields. (trigger: tests/test_run_command_stdout.py) (restates: Axis 3: Payload placeholder/null field contract asserted for success_criteria_results only)

Downstream consumers:

• 🔵 Downstream scoring-script authors consuming the context JSON are told duration_seconds is null, but EvaluationResult.duration_seconds is a non-nullable float defaulting to 0.0 — a script trusting the doc (e.g. if ctx['duration_seconds'] is None) misreads the payload. The field description and docs/TASK_DEFINITION_GUIDE.md should say 0.0 (not finalized) rather than null. (trigger: docs/TASK_DEFINITION_GUIDE.md) (restates: Axis 7/8: pass_context docs claim duration_seconds is null but it defaults to 0.0)

Harness & Lint Improvements

Static checks (lint / type):

• [ce-lint] Add CE026 (next free number, wired into tests/lint/rules/ + tests/lint/runner.py) enforcing the pass_context 'provisional field' contract from a single source of truth: define one canonical PROVISIONAL_FIELDS constant listing every EvaluationResult field not finalized until after the criteria phase (weighted_score, completed_at, duration_seconds, final_status, success_criteria_results), and forbid _serialize_context (criteria/run_command.py:35) from calling model_copy(update={...}) with an update-dict whose keys are not exactly that set. This is the same 'two sites must stay consistent' shape as CE025 (live_stop_polarities <-> live_verdict). Root cause of both A8-medium and A7/A8-low is that the scrub subset and the documented null-list are hand-maintained in two places and drifted; a rule anchoring the scrub to the constant makes an omitted field (weighted_score) a mechanical lint failure. Prevents: A8-medium (stale weighted_score leaks in dialog mode because the scrub omits it) and, once the docstring is generated/checked against the same constant, the A7/A8-low doc-vs-model mismatch (duration_seconds documented as null).
• [pyright] Tighten the documented contract into the type system where possible: the A7/A8-low finding is that duration_seconds is declared float = 0.0 (non-nullable) yet documented as 'null'. Rather than a doc-only fix, consider the scrub resetting it to a sentinel that matches the type (0.0), and add a @override/typed helper so the placeholder-payload shape is a named typed object rather than an ad-hoc model_copy — making the 'these fields are provisional' invariant visible to pyright instead of living only in a prose docstring. Prevents: A7/A8-low (duration_seconds documented as null but is a non-nullable float defaulting to 0.0).

Harness improvements (not statically reachable):

• Add a golden/contract test that runs the real orchestrator through a pass_context run_command criterion in BOTH single-shot and simulation every_turn/both modes, deserializes the payload the scoring script actually saw at $CODER_EVAL_CONTEXT, and asserts every field of the documented contract: success_criteria_results == [], weighted_score is None, completed_at is None, duration_seconds == 0.0, final_status is the placeholder. The dialog variant must run at least two turns so turn N's calculate_weighted_score has fired before turn N+1's check. Why not static: The A8-medium leak only manifests at runtime: it depends on orchestrator._run_dialog_criteria_check calling calculate_weighted_score at the end of turn N, mutating shared state that a static reader of _serialize_context cannot see. A static check can enforce the scrub-set shape but not that the mutation actually leaked mid-dialog. Prevents: A8-medium (stale prior-turn weighted_score), A3-low (only success_criteria_results asserted, not weighted_score/completed_at/duration_seconds), and A7/A8-low (duration_seconds actual value).
• Add one orchestrator-level test that drives check_success/check_all through the production seams (orchestrator.py:1355/1415/1509, each passing run_result=self.result) with a pass_context run_command criterion, and asserts the criterion did not raise ValueError and the script observed a populated context — guarding the trivial attribute-passing that every current test bypasses by calling SuccessChecker.check/check_all directly. Why not static: run_result is an optional kwarg; dropping run_result=self.result from a call site is not a type error and not grep-detectable as a defect (the call still type-checks and other callers legitimately omit it). Only a runtime assertion that the script sees non-error context proves the seam is wired. Prevents: A3-low (orchestrator pass_context wiring untested at all three call sites).

Top 5 Priority Actions

1. Fix the dialog-mode stale-weighted_score leak — extend _serialize_context (src/coder_eval/criteria/run_command.py:35) to also reset weighted_score=None (the one provisional field actually populated mid-dialog by _run_dialog_criteria_check) so the serialized $CODER_EVAL_CONTEXT payload matches its documented "null — do not read" contract in every_turn/both simulation mode, not just single-shot. This is the only correctness/reproducibility issue in the PR.
2. Add a simulation every_turn dialog-mode pass_context test (tests/test_pass_context.py) that runs ≥2 turns and asserts the serialized context's weighted_score is null at turn N+1 regardless of turn N's score — the exact path where the leak lives and currently ships unguarded.
3. Correct the docs: duration_seconds is documented as null (src/coder_eval/models/criteria.py:236 and docs/TASK_DEFINITION_GUIDE.md) but is a non-nullable float defaulting to 0.0; change both to say 0.0 (not finalized) so script authors don't test is None.
4. Add one orchestrator-level test covering the three run_result=self.result wiring seams (src/coder_eval/orchestrator.py:1355 / 1415 / 1509) so a dropped kwarg can't silently make every pass_context criterion raise ValueError and score 0.0 in production with no failing test.
5. State the PATH-change nightly blast radius explicitly — the prepend→append of <sandbox>/node_modules/.bin in _build_run_command_env (src/coder_eval/sandbox.py) alters binary resolution on the production/nightly scoring path; confirm no nightly task relies on node_modules/.bin shadowing. Consider adopting the proposed CE026 lint rule anchoring the _serialize_context scrub to a single canonical PROVISIONAL_FIELDS constant to prevent the scrub/doc-drift class outright.

Stats: 0 🔴 · 0 🟠 · 1 🟡 · 4 🔵 across 8 axes reviewed.